Skip to content

Test.refactor.coverage #297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Conversation

filipecosta90
Copy link
Collaborator

to be discussed after #296

@filipecosta90 filipecosta90 requested a review from rafie February 20, 2020 19:14
@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #297 into master will increase coverage by 1.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #297     +/-   ##
=========================================
+ Coverage   51.81%   52.92%   +1.1%     
=========================================
  Files          25       25             
  Lines        4624     4631      +7     
=========================================
+ Hits         2396     2451     +55     
+ Misses       2228     2180     -48
Impacted Files Coverage Δ
src/backends/torch.c 82.64% <100%> (+0.29%) ⬆️
src/redisai.c 75.93% <100%> (+1.53%) ⬆️
src/backends.c 54.95% <0%> (+1.65%) ⬆️
src/tensor.c 78.78% <0%> (+11.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4709f0...35fd661. Read the comment docs.

@@ -22,7 +22,7 @@ def common_first(self):

if self.os == 'linux':
self.install("ca-certificates")
self.install("git cmake unzip wget patchelf awscli")
self.install("git cmake unzip wget patchelf awscli valgrind")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail on macOS. Better to install Valgrind in:

def linux(self):
    self.install("valgrind")

@rafie
Copy link
Contributor

rafie commented Feb 25, 2020

This can be way off the current implementation, but as we don't use CMake to run tests, I prefer to drive coverage data collection and report generation outside CMake (the build part is of course ok but it's relatively modest in configuration). TimeSeries has it implemented this way.

@filipecosta90
Copy link
Collaborator Author

This can be way off the current implementation, but as we don't use CMake to run tests, I prefer to drive coverage data collection and report generation outside CMake (the build part is of course ok but it's relatively modest in configuration). TimeSeries has it implemented this way.

Agree @rafie. will check TS and change accordingly :)

@filipecosta90
Copy link
Collaborator Author

closing in favor of #299

@filipecosta90 filipecosta90 deleted the test.refactor.coverage branch April 9, 2020 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants